Retry GitHub download failures #3729
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Does this even look in the ballpark? My Python is not good 😄 |
leahwicz
commented
Aug 11, 2021
jtcohen6
reviewed
Aug 11, 2021
There was a problem hiding this comment.
Hooray!! Glad we're reusing + extending the existing decorator. The code looks pretty good to me, but I should not be the arbiter :)
Also interested to hear folks' thoughts about how best to test - perhaps mocking a package with a nonexistent tarball URL? I know test_registry_get_request_exception added some unneeded baggage to our unit testing.
There was a problem hiding this comment.
The python checks out. However, on a personal note, I find this wrapping pattern to be particularly opaque.
Here is an example of how I might suggest refactoring this:
# no juggling of indexes which is extremely error prone
def _exception_retry(lambda_fn, retries):
if retries <= 0:
raise RegistryException('Unable to connect to registry hub')
else:
try:
return lambda_fn()
except (requests.exceptions.ConnectionError, requests.exceptions.Timeout):
time.sleep(1)
_exception_retry(lambda_fn, retries - 1)
# example call:
# accepting a lambda function as a parameter allows the outer
# function to run it regardless of what parameters it takes
_exception_retry(lambda: 1+2, 5)(the original exception is lost here, but that can be put back. I didn't write that part because it obfuscated the point I'm trying to make in the review)
In both ways of writing this, I would suggest the following tests:
- pass a function that always throws these exceptions and check that you get the right exception back
- pass a function that throws twice then returns correctly and check that you get the correct response
gshank
reviewed
Aug 13, 2021
Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com>
kwigley
approved these changes
Aug 24, 2021
leahwicz
added a commit
that referenced
this pull request
Aug 24, 2021
* Retry GitHub download failures * Refactor and add tests * Fixed linting and added comment * Fixing unit test assertRaises Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Fixing casing Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Changing to use partial for function calls Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com>
4 tasks
leahwicz
added a commit
that referenced
this pull request
Aug 27, 2021
* Retry GitHub download failures * Refactor and add tests * Fixed linting and added comment * Fixing unit test assertRaises Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Fixing casing Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Changing to use partial for function calls Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com>
IS-Josh
pushed a commit
to IS-Josh/dbt
that referenced
this pull request
Sep 4, 2021
* Retry GitHub download failures * Refactor and add tests * Fixed linting and added comment * Fixing unit test assertRaises Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Fixing casing Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Changing to use partial for function calls Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com>
TeddyCr
pushed a commit
to TeddyCr/dbt
that referenced
this pull request
Sep 9, 2021
* Retry GitHub download failures * Refactor and add tests * Fixed linting and added comment * Fixing unit test assertRaises Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Fixing casing Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Changing to use partial for function calls Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com>
4 tasks
iknox-fa
pushed a commit
that referenced
this pull request
Feb 8, 2022
* Retry GitHub download failures * Refactor and add tests * Fixed linting and added comment * Fixing unit test assertRaises Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Fixing casing Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> * Changing to use partial for function calls Co-authored-by: Kyle Wigley <kyle@fishtownanalytics.com> automatic commit by git-black, original commits: 09ea989
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #3546
Description
We are seeing
dbt depsfail intermittently when trying to reach out to GitHub. We do retry logic for when we reach out to the registry hub so we want to try the same retry logic for GitHub to see if it improves the success rate ofdbt deps.Checklist
CHANGELOG.mdand added information about my change to the "dbt next" section.